Rollup: Specify AMD id #8843

nicjansma · 2024-08-26T14:27:50Z

Description

Today, video.js exports itself as an AMD module (if the site supports AMD) via the UMD header:

(function (global, factory) {
  typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
  typeof define === 'function' && define.amd ? define(factory) :
  (global = typeof globalThis !== 'undefined' ? globalThis : global || self, global.videojs = factory());
})(this, (function () { 'use strict';

The second clause which runs if AMD/RequireJS is active, is a define(factory) function call. This is an "anonymous" definition of the module. Being anonymous will break on sites using the lightweight RequireJS shim called almond, which expect all modules to be named.

This can be challenging if video.js is being pulled into the site via a third-party, i.e. as part of ads.

Prior to using Browserify/Rollup, video.js had implemented a change to ensure define('videojs', factory) was used:

#1844

However, after the switch to Browserify, that small change was lost:

#3826

This error is seen by others, i.e. #1843 (comment)

Specific Changes proposed

This PR simply ensures video.js is named for AMD via the rollup.config.js.

The UMD definition changes to this:

(function (global, factory) {
  typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
  typeof define === 'function' && define.amd ? define('videojs', factory) :
  (global = typeof globalThis !== 'undefined' ? globalThis : global || self, global.videojs = factory());
})(this, (function () { 'use strict';

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chrome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
- Has no DOM changes which impact accessiblilty or trigger warnings (e.g. Chrome issues tab)
- Has no changes to JSDoc which cause npm run docs:api to error
Reviewed by Two Core Contributors

welcome · 2024-08-26T14:27:53Z

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

Run npm run lint -- --errors locally to catch formatting errors earlier.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

gkatsev · 2024-08-26T15:05:03Z

Can this change break folks that are currently using AMD? I think that would be my only concern with a change like this.

nicjansma · 2024-08-26T18:41:27Z

@gkatsev I don't believe this is a breaking change at all. This is how video.js was until ~6.2.* as well.

The Rollup code just uses that new id option, and makes sure the generated code has the name specified.

gkatsev · 2024-08-26T19:07:36Z

Sure, but if someone has amd set up and working with 8.17.3, and we release this change, would they need to change their configuration for it to work again or would it not matter and just would mean less configuration for folks going forward again?

codecov · 2024-08-27T14:54:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.79%. Comparing base (f0db8f1) to head (bdd3f39).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8843      +/-   ##
==========================================
+ Coverage   83.18%   83.79%   +0.60%     
==========================================
  Files         120      120              
  Lines        8047     8047              
  Branches     1931     1931              
==========================================
+ Hits         6694     6743      +49     
+ Misses       1353     1304      -49

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nicjansma · 2024-08-28T00:06:28Z

I wasn't sure about this until I built a bunch of test cases! Here's what I've found:

Using define(factory) (existing behavior):

✅ video.js direct inclusion
✅ video.js with RequireJS loaded as require(['lib/videojs'], ...)
✅ video.js with RequireJS loaded as require(['videojs'], ...) (require path override specified)
✅ video.js with RequireJS loaded as require(['lib/videojs'], ...) using r.js optimizer
✅ video.js with RequireJS loaded as require(['videojs'], ...) using r.js optimizer (require path override specified)
✅ video.js with RequireJS loaded as require(['lib/videojs'], ...) using almond
✅ video.js with RequireJS loaded as require(['videojs'], ...) using almond (require path override specified)

Using define('videojs', factory) (this PR):

So unfortunately it looks like by forcing the module id to 'videojs' in the define(...), it will break existing cases where video.js is being included at a specified (relative) path such as require(['lib/videojs'])

<html>
    <head>
        <link href="lib/video-js.css" rel="stylesheet">
        <script data-main="main" src="lib/require.js"></script>
        <!-- video.js is at lib/videojs.js -->
    </head>
    <body>
        <script>
            require(['lib/videojs'], function(videojs) {
                // videojs will be undefined if built with the AMD id of `videojs`
        </script>
    </body>
</html>

In these cases, the file lib/videojs.js is fetched, but the file itself only defines videojs. So callback var of the require() statement is undefined.

The cases where it continues to work are where the user had already specified the module as videojs and just used a RequireJS path override to point to where that file is, e.g. lib/videojs.js.

<html>
    <head>
        <link href="lib/video-js.css" rel="stylesheet">
        <script data-main="main" src="lib/require.js"></script>
        <!-- video.js is at lib/videojs.js -->
    </head>
    <body>
        <script>
            requirejs.config({
                paths: {
                    'videojs': 'lib/videojs'
                },
            });
            require(['videojs'], function(videojs) {
                // videojs is OK here
        </script>
    </body>
</html>

So I'm not sure where that leaves this PR. This would potentially be a breaking change for some.

It would be nice to fix this though. I came across this issue when my ad provider, who is using video.js, started to include it in their bundle. On my site that uses Almond (which requires a name for all modules), video.js wouldn't load.

gkatsev · 2024-08-28T01:57:03Z

we could provide another build that includes it, but the main build would need to wait for a breaking change.

Rollup specify AMD id

bdd3f39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollup: Specify AMD id #8843

Rollup: Specify AMD id #8843

nicjansma commented Aug 26, 2024 •

edited

Loading

welcome bot commented Aug 26, 2024

gkatsev commented Aug 26, 2024

nicjansma commented Aug 26, 2024 •

edited

Loading

gkatsev commented Aug 26, 2024

codecov bot commented Aug 27, 2024 •

edited

Loading

nicjansma commented Aug 28, 2024

gkatsev commented Aug 28, 2024

Rollup: Specify AMD id #8843

Are you sure you want to change the base?

Rollup: Specify AMD id #8843

Conversation

nicjansma commented Aug 26, 2024 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

welcome bot commented Aug 26, 2024

gkatsev commented Aug 26, 2024

nicjansma commented Aug 26, 2024 • edited Loading

gkatsev commented Aug 26, 2024

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

nicjansma commented Aug 28, 2024

gkatsev commented Aug 28, 2024

nicjansma commented Aug 26, 2024 •

edited

Loading

nicjansma commented Aug 26, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading